feat(collection-scripts): backend execution engine for collection hooks#7872
Open
Kniggishood wants to merge 3 commits intousebruno:mainfrom
Open
feat(collection-scripts): backend execution engine for collection hooks#7872Kniggishood wants to merge 3 commits intousebruno:mainfrom
Kniggishood wants to merge 3 commits intousebruno:mainfrom
Conversation
Adds the electron-side foundation for running shell scripts registered on a collection: a security-checked spawn helper (executes the script directly so its shebang picks the interpreter), a KEY=VALUE stdout parser for env-var injection, an IPC handler gated behind Developer Mode, and a beforeCollectionRun integration in the runner. The bruno schema gains a collectionScript entry so configs round-trip safely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Kniggishood
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughAdds collection script support: IPC handler for renderer-initiated script runs, utilities to safely execute and parse shell script output, pre-run execution of collection scripts before collection runs (when sandbox mode is developer), schema additions, and tests covering utilities and IPC validation. No exported API surface changes beyond new module export. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant MainIPC as Main IPC
participant NetworkRunner as Network Runner
participant ScriptExec as Script Executor
participant FS as File System
rect rgba(100,150,200,0.5)
note over Renderer,FS: Pre-Run Script Execution (collection run)
NetworkRunner->>NetworkRunner: check jsSandboxMode === 'developer'
NetworkRunner->>NetworkRunner: select scripts with runOn includes 'beforeCollectionRun'
loop each script
NetworkRunner->>ScriptExec: runShellScript(collectionPath, scriptFile)
ScriptExec->>FS: resolve & validate script path
ScriptExec->>FS: spawn process (cwd = collection)
FS-->>ScriptExec: stdout/stderr chunks
ScriptExec->>NetworkRunner: return {stdout, stderr, exitCode, truncated}
alt exitCode === 0 and outputMode === 'envVars'
NetworkRunner->>NetworkRunner: parseEnvVarsFromOutput(stdout)
NetworkRunner->>MainIPC: emit 'main:script-environment-update'
MainIPC->>Renderer: notify env / persistent updates
else
NetworkRunner->>MainIPC: log error / emit stderr chunks
end
end
NetworkRunner->>NetworkRunner: proceed with collection run
end
sequenceDiagram
participant Renderer
participant MainIPC as Main IPC
participant ScriptExec as Script Executor
participant FS as File System
rect rgba(150,100,200,0.5)
note over Renderer,FS: Renderer-initiated script execution
Renderer->>MainIPC: renderer:run-collection-script {collectionPath, script}
MainIPC->>MainIPC: validate payload & jsSandboxMode === 'developer'
MainIPC->>MainIPC: isScriptPathSafe(collectionPath, script.file)?
alt invalid
MainIPC-->>Renderer: error response
else valid
MainIPC->>ScriptExec: runShellScript(collectionPath, script.file)
ScriptExec->>FS: spawn process
loop while running
FS-->>ScriptExec: stdout chunk
ScriptExec->>MainIPC: emit 'main:collection-script-output' (stdout)
MainIPC-->>Renderer: stream stdout
FS-->>ScriptExec: stderr chunk
ScriptExec->>MainIPC: emit 'main:collection-script-output' (stderr)
MainIPC-->>Renderer: stream stderr
end
FS-->>ScriptExec: process exits
ScriptExec-->>MainIPC: {exitCode, stdout, stderr}
alt outputMode === 'envVars' && exitCode === 0
MainIPC->>MainIPC: parseEnvVarsFromOutput(stdout) & merge
MainIPC->>Renderer: 'main:script-environment-update', 'main:persistent-env-variables-update'
end
MainIPC-->>Renderer: response with exitCode & outputs
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a backend execution engine in bruno-electron for running collection hook scripts (with path safety checks, streaming output over IPC, and optional env-var injection), plus supporting schema/test updates.
Changes:
- Introduces
isScriptPathSafe,runShellScript, andparseEnvVarsFromOutpututilities (with unit tests). - Adds a new IPC handler to execute collection scripts and stream stdout/stderr to the renderer.
- Runs
beforeCollectionRunscripts as part of the collection runner and merges parsed env vars into the active env.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/bruno-schema/src/collections/index.js | Adds collectionScriptSchema and exports it from the collections schema module. |
| packages/bruno-electron/src/utils/collection-scripts.js | New utilities for path validation, script execution (spawn), and env-var parsing from stdout. |
| packages/bruno-electron/src/ipc/scripts.js | New IPC handler to run a script and stream output to the renderer; applies security gating. |
| packages/bruno-electron/src/ipc/network/index.js | Wires beforeCollectionRun hook execution into the folder/collection runner. |
| packages/bruno-electron/src/index.js | Registers the new collection scripts IPC module at app startup. |
| packages/bruno-electron/tests/utils/collection-scripts.spec.js | Adds unit tests for path safety, env-var parsing, and streaming behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+34
| function parseEnvVarsFromOutput(output) { | ||
| const vars = {}; | ||
| for (const line of output.split('\n')) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed || trimmed.startsWith('#')) continue; | ||
| const match = trimmed.match(/^(?:export\s+)?([A-Za-z_][A-Za-z0-9_]*)=(.*)$/); | ||
| if (match) { | ||
| vars[match[1]] = match[2]; | ||
| } | ||
| } | ||
| return vars; | ||
| } |
Comment on lines
+47
to
+51
| function runShellScript(collectionPath, scriptFile, { onStdout, onStderr } = {}) { | ||
| return new Promise((resolve, reject) => { | ||
| const resolvedScript = path.resolve(collectionPath, scriptFile); | ||
| const proc = spawn(resolvedScript, [], { cwd: collectionPath, env: process.env }); | ||
| let stdout = ''; |
Comment on lines
+8
to
+16
| ipcMain.handle('renderer:run-collection-script', async (event, { collectionUid, collectionPath, script }) => { | ||
| const securityConfig = collectionSecurityStore.getSecurityConfigForCollection(collectionPath); | ||
| if (securityConfig?.jsSandboxMode !== 'developer') { | ||
| return { error: 'Collection scripts require Developer Mode. Enable it in Collection Settings > Security.' }; | ||
| } | ||
|
|
||
| if (!isScriptPathSafe(collectionPath, script.file)) { | ||
| return { error: 'Script path must be within the collection directory.' }; | ||
| } |
| // Results are merged into envVars so the runner picks them up immediately. | ||
| const securityConfig = collectionSecurityStore.getSecurityConfigForCollection(collectionPath); | ||
| if (securityConfig?.jsSandboxMode === 'developer') { | ||
| const beforeRunScripts = get(brunoConfig, 'collectionScripts', []).filter( |
| }) | ||
| .noUnknown(true) | ||
| .strict(); | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/bruno-electron/tests/utils/collection-scripts.spec.js (1)
6-34: Add a symlink-escape regression test for path safety.Current
isScriptPathSafetests cover..and prefix tricks, but notscriptFilepaths that are symlinks inside the collection pointing outside. That edge case should be covered.As per coding guidelines: cover both happy paths and realistically problematic paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/tests/utils/collection-scripts.spec.js` around lines 6 - 34, Add a regression test to verify isScriptPathSafe rejects symlink-escaped paths: create a real temp directory outside the collection, inside the collection create a symlink file (or directory) that points to that external path and assert isScriptPathSafe(collectionPath, symlinkPathRelativeOrAbsolute) returns false; use Node fs.symlinkSync (or fs.promises.symlink) to create the symlink and clean up after the test; reference isScriptPathSafe in the new test and add it alongside the existing cases in collection-scripts.spec.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-electron/src/ipc/network/index.js`:
- Around line 1341-1354: When runShellScript returns a non-zero exitCode for a
beforeCollectionRun script, the current code just skips parsing and swallows the
failure; update the block after awaiting runShellScript(script.file) to detect
exitCode !== 0 and emit/log the failure (include script.name, exitCode and
stderr) and then continue to next script; keep existing behavior for exitCode
=== 0 (parseEnvVarsFromOutput, Object.assign(envVars), and the
mainWindow.webContents.send updates) but ensure failures are visible by calling
console.error (or an IPC failure event via mainWindow.webContents.send) with the
stderr and exitCode so hook failures are not silent.
In `@packages/bruno-electron/src/ipc/scripts.js`:
- Around line 8-15: In the ipcMain.handle('renderer:run-collection-script')
handler ensure you validate the incoming payload before accessing script.file:
check that collectionPath is a non-empty string, script is an object, and
script.file is a string (and return the same error-object shape if any check
fails) before calling collectionSecurityStore.getSecurityConfigForCollection or
isScriptPathSafe; update the handler around the existing references to
collectionPath, script.file,
collectionSecurityStore.getSecurityConfigForCollection and isScriptPathSafe to
perform these guards and return a controlled { error: '...' } response for
malformed payloads.
In `@packages/bruno-electron/src/utils/collection-scripts.js`:
- Around line 51-63: The current stdout/stderr accumulation in the
proc.stdout.on('data') and proc.stderr.on('data') handlers (variables stdout,
stderr and callbacks onStdout/onStderr) is unbounded and can OOM; add a
configurable max buffer (e.g., maxBufferBytes = 1MB) and track bytesAppended per
stream, appending only up to that limit and setting a truncated flag or counter
when more data arrives; continue to invoke onStdout/onStderr with each chunk but
avoid growing the stdout/stderr strings past the cap (you can keep the last N
bytes or the first N bytes and a "...(truncated)" marker), and ensure the final
resolve({ exitCode, stdout, stderr }) reflects the capped content and any
truncated indicator so callers know output was truncated.
- Line 50: The spawn call using resolvedScript (in the module that runs
collection scripts) assumes Unix shebang handling; modify the logic around the
spawn invocation so it checks isWindowsOS() and, on Windows, runs the script via
the shell (pass shell: true in the spawn options) or explicitly reject
non-executable shebang scripts with a clear error; update the spawn options to
include cwd: collectionPath and env: process.env plus conditional shell: true
when isWindowsOS() returns true, and keep the existing behavior for non-Windows
platforms.
- Around line 11-15: The isScriptPathSafe function currently uses path.resolve
(lexical) which doesn't dereference symlinks; update it to call fs.realpathSync
on the collectionPath and the resolved script path (e.g., realCollection =
fs.realpathSync(collectionPath) and realScript =
fs.realpathSync(path.resolve(collectionPath, scriptFile))) before doing the
equality/startsWith check so symlink targets are compared against the real
collection boundary; wrap the realpath calls in a try/catch and return false on
error to avoid allowing unsafe paths.
In `@packages/bruno-electron/tests/utils/collection-scripts.spec.js`:
- Around line 132-159: Tests assume /bin/bash and Unix shell semantics (in
writeScript usage for 'hello.sh', 'err.sh', 'plain.sh') which fails on Windows;
modify the suite to detect availability of a POSIX shell before running these
tests and skip them deterministically when not present. Update the tests around
runShellScript/writeScript to either (a) check for a usable shell (e.g.,
spawnSync('bash','--version') or whichSync('bash')) in a beforeAll and call
test.skip or return early when absent, or (b) rewrite the scripts to use a
cross-platform Node-based script invoked via node so they don't rely on
/bin/bash; ensure changes reference the existing helpers runShellScript and
writeScript so call sites remain consistent.
---
Nitpick comments:
In `@packages/bruno-electron/tests/utils/collection-scripts.spec.js`:
- Around line 6-34: Add a regression test to verify isScriptPathSafe rejects
symlink-escaped paths: create a real temp directory outside the collection,
inside the collection create a symlink file (or directory) that points to that
external path and assert isScriptPathSafe(collectionPath,
symlinkPathRelativeOrAbsolute) returns false; use Node fs.symlinkSync (or
fs.promises.symlink) to create the symlink and clean up after the test;
reference isScriptPathSafe in the new test and add it alongside the existing
cases in collection-scripts.spec.js.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 955f7803-f825-4ae5-870f-dbf37907948e
📒 Files selected for processing (6)
packages/bruno-electron/src/index.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/ipc/scripts.jspackages/bruno-electron/src/utils/collection-scripts.jspackages/bruno-electron/tests/utils/collection-scripts.spec.jspackages/bruno-schema/src/collections/index.js
…rdening Addresses CodeRabbit review on usebruno#7872: - isScriptPathSafe now resolves through symlinks via fs.realpathSync, so a symlink inside the collection pointing outside it is rejected. Lexical fallback is preserved when the collection path doesn't exist on disk so unit tests with fictional paths still work; missing scripts return false. - runShellScript caps stdout/stderr accumulation at maxBuffer (default 1 MiB) per stream and marks truncated=true so a runaway script can't exhaust the Electron main process. - buildSpawnArgs centralises the platform-specific spawn shape: shell:true + windowsHide on win32 (so shebangs/.cmd/.bat/.js file associations work), direct invocation on POSIX where the kernel honours the shebang. - Tests rewritten to use #\!/usr/bin/env node script bodies instead of bash, so the suite no longer assumes /bin/bash. Adds buildSpawnArgs unit tests, a symlink-escape regression test, and maxBuffer truncation tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…xits Addresses CodeRabbit review on usebruno#7872: - ipc/scripts.js now type-checks collectionPath, script, and script.file before doing anything else, so a malformed renderer payload returns a structured error instead of throwing inside the handler. - network/index.js no longer silently swallows non-zero exits from beforeCollectionRun hooks. Failures are logged with the script name, exit code, and a stderr tail, and forwarded to the renderer over the existing main:collection-script-output channel so the user can see them. - Adds tests/ipc/scripts.spec.js covering each validation branch and the developer-mode gate, with electron / store / utils mocked. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/bruno-electron/tests/ipc/scripts.spec.js (2)
40-80: Add tests for remaining validation branches (''and non-object payload forms).The handler also rejects
collectionPath: '',scriptas a non-object, andscript.file: ''. Adding these closes the remaining payload-validation gaps.Suggested test additions
+ test('rejects when collectionPath is an empty string', async () => { + const result = await handler({}, { + collectionUid: 'c1', + collectionPath: '', + script: { uid: 's', file: 'run.sh' } + }); + expect(result.error).toMatch(/collection path/i); + expect(mockRunShellScript).not.toHaveBeenCalled(); + }); + + test('rejects when script is not an object', async () => { + const result = await handler({}, { + collectionUid: 'c1', + collectionPath: '/c', + script: 'run.sh' + }); + expect(result.error).toMatch(/script/i); + expect(mockRunShellScript).not.toHaveBeenCalled(); + }); + + test('rejects when script.file is an empty string', async () => { + const result = await handler({}, { + collectionUid: 'c1', + collectionPath: '/c', + script: { uid: 's', file: '' } + }); + expect(result.error).toMatch(/script\.file|script file/i); + expect(mockRunShellScript).not.toHaveBeenCalled(); + });As per coding guidelines, “Cover both the "happy path" and the realistically problematic paths. Validate expected success behaviour, but also validate error handling, edge cases, and degraded-mode behaviour when appropriate.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/tests/ipc/scripts.spec.js` around lines 40 - 80, Add three unit tests to exercise the remaining validation branches in the handler: (1) collectionPath set to '' should be rejected and mockRunShellScript must not be called, (2) script provided as a non-object (e.g. a string or number) should be rejected and mockRunShellScript must not be called, and (3) script.file set to '' should be rejected and mockRunShellScript must not be called. Follow the existing test patterns around handler(...) and assertions on result.error (use regex that matches "collection path" or "script" / "script.file") and ensure each test calls expect(mockRunShellScript).not.toHaveBeenCalled() to verify no execution occurred.
35-36: Harden handler lookup to fail with a clearer message.If registration regresses,
call[1]throws a generic TypeError. Add an explicit assertion before dereferencing for easier diagnosis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/tests/ipc/scripts.spec.js` around lines 35 - 36, The lookup of the registered handler is fragile because call may be undefined before dereferencing call[1]; update the test around mockHandle.mock.calls.find((c) => c[0] === 'renderer:run-collection-script') to assert the result exists before extracting the handler (e.g. using an explicit assertion like expect(call).toBeDefined() or throwing a clear Error if not found), then set handler = call[1]; this ensures a meaningful failure message if the registration regresses (reference mockHandle, the find call, the 'renderer:run-collection-script' key, and the handler variable).packages/bruno-electron/tests/utils/collection-scripts.spec.js (1)
53-58: Narrow the symlink setup catch so real failures don’t get silently skipped.Line 53-Line 58 currently treats any
fs.symlinkSyncerror as "unsupported". This can hide real regressions. Prefer skipping only known capability errors (for exampleEPERM/EACCES/ENOSYS/EINVAL), and rethrow others.Proposed fix
try { fs.symlinkSync(path.join(outsideDir, 'evil.sh'), path.join(collectionDir, 'link.sh')); } catch (err) { - // Windows non-admin can't create symlinks — skip rather than fail. - symlinkSupported = false; + // Windows non-admin or restricted environments may block symlink creation. + if (['EPERM', 'EACCES', 'ENOSYS', 'EINVAL'].includes(err.code)) { + symlinkSupported = false; + } else { + throw err; + } }Based on learnings: tests should "Aim for tests that fail usefully. When a test fails, it should clearly indicate what behaviour broke and why."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/tests/utils/collection-scripts.spec.js` around lines 53 - 58, The symlink setup swallow-all catch masks real failures; change the try/catch around fs.symlinkSync (the call creating path.join(collectionDir, 'link.sh') from path.join(outsideDir, 'evil.sh')) to only suppress known platform capability errors (EPERM, EACCES, ENOSYS, EINVAL) by checking err.code and setting symlinkSupported = false for those codes, but rethrow the error for any other err.code so real regressions surface. Ensure the catch references the same fs.symlinkSync invocation and symlinkSupported variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-electron/tests/utils/collection-scripts.spec.js`:
- Around line 201-208: The test comment says a .cmd shim is written but
writeNodeScript only creates the .js file; update writeNodeScript to also emit a
Windows shim file (e.g., same basename + ".cmd") next to the JS file that
forwards arguments to node (for example: `@echo` off\r\nnode
"%~dp0\\<filename>.js" %*), write it into tmpDir alongside the .js and ensure
both files are created (use the existing fullPath and filename to derive the
shim name), so Windows CI won't rely on system-level .js associations; keep
existing permissions behavior for the .js file.
---
Nitpick comments:
In `@packages/bruno-electron/tests/ipc/scripts.spec.js`:
- Around line 40-80: Add three unit tests to exercise the remaining validation
branches in the handler: (1) collectionPath set to '' should be rejected and
mockRunShellScript must not be called, (2) script provided as a non-object (e.g.
a string or number) should be rejected and mockRunShellScript must not be
called, and (3) script.file set to '' should be rejected and mockRunShellScript
must not be called. Follow the existing test patterns around handler(...) and
assertions on result.error (use regex that matches "collection path" or "script"
/ "script.file") and ensure each test calls
expect(mockRunShellScript).not.toHaveBeenCalled() to verify no execution
occurred.
- Around line 35-36: The lookup of the registered handler is fragile because
call may be undefined before dereferencing call[1]; update the test around
mockHandle.mock.calls.find((c) => c[0] === 'renderer:run-collection-script') to
assert the result exists before extracting the handler (e.g. using an explicit
assertion like expect(call).toBeDefined() or throwing a clear Error if not
found), then set handler = call[1]; this ensures a meaningful failure message if
the registration regresses (reference mockHandle, the find call, the
'renderer:run-collection-script' key, and the handler variable).
In `@packages/bruno-electron/tests/utils/collection-scripts.spec.js`:
- Around line 53-58: The symlink setup swallow-all catch masks real failures;
change the try/catch around fs.symlinkSync (the call creating
path.join(collectionDir, 'link.sh') from path.join(outsideDir, 'evil.sh')) to
only suppress known platform capability errors (EPERM, EACCES, ENOSYS, EINVAL)
by checking err.code and setting symlinkSupported = false for those codes, but
rethrow the error for any other err.code so real regressions surface. Ensure the
catch references the same fs.symlinkSync invocation and symlinkSupported
variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf3aab6b-1f25-4609-9787-8f68f5fae3a6
📒 Files selected for processing (5)
packages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/ipc/scripts.jspackages/bruno-electron/src/utils/collection-scripts.jspackages/bruno-electron/tests/ipc/scripts.spec.jspackages/bruno-electron/tests/utils/collection-scripts.spec.js
✅ Files skipped from review due to trivial changes (1)
- packages/bruno-electron/src/ipc/scripts.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-electron/src/utils/collection-scripts.js
- packages/bruno-electron/src/ipc/network/index.js
Comment on lines
+201
to
+208
| // Cross-platform: shebang dispatches to whatever `node` is on PATH (POSIX). | ||
| // On Windows, runShellScript uses shell:true so the .js extension routes to | ||
| // the Node file association. We additionally write a `.cmd` shim there. | ||
| const writeNodeScript = (filename, jsBody) => { | ||
| const fullPath = path.join(tmpDir, filename); | ||
| fs.writeFileSync(fullPath, `#!/usr/bin/env node\n${jsBody}\n`, { mode: 0o755 }); | ||
| return filename; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the actual content of the test file around the disputed lines
head -n 250 packages/bruno-electron/tests/utils/collection-scripts.spec.js | tail -n +195Repository: usebruno/bruno
Length of output: 2087
🏁 Script executed:
# Find and examine runShellScript implementation
fd -e js -e ts --type f | xargs grep -l "runShellScript" | head -5Repository: usebruno/bruno
Length of output: 470
🏁 Script executed:
# Search for the runShellScript function definition
rg "const runShellScript|function runShellScript" -A 20 --type jsRepository: usebruno/bruno
Length of output: 2082
🏁 Script executed:
# Find buildSpawnArgs implementation
rg "buildSpawnArgs" -A 30 packages/bruno-electron/src/utils/collection-scripts.jsRepository: usebruno/bruno
Length of output: 1750
Comment claims .cmd shim is written, but code doesn't write one—Windows execution becomes environment-dependent.
The comment at lines 201–203 states "We additionally write a .cmd shim there," but writeNodeScript only writes the .js file. On Windows, execution relies on system-level .js file association rather than an explicit shim, making tests fragile in CI environments without guaranteed Node.js file associations.
Proposed fix
const writeNodeScript = (filename, jsBody) => {
const fullPath = path.join(tmpDir, filename);
fs.writeFileSync(fullPath, `#!/usr/bin/env node\n${jsBody}\n`, { mode: 0o755 });
- return filename;
+ if (process.platform === 'win32') {
+ const cmdName = `${path.parse(filename).name}.cmd`;
+ const cmdPath = path.join(tmpDir, cmdName);
+ fs.writeFileSync(
+ cmdPath,
+ `@echo off\r\n"${process.execPath}" "%~dp0${filename}" %*\r\n`
+ );
+ return cmdName;
+ }
+ return filename;
};Per test standards: tests must be deterministic with no environment-specific assumptions without explicit control.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/tests/utils/collection-scripts.spec.js` around lines
201 - 208, The test comment says a .cmd shim is written but writeNodeScript only
creates the .js file; update writeNodeScript to also emit a Windows shim file
(e.g., same basename + ".cmd") next to the JS file that forwards arguments to
node (for example: `@echo` off\r\nnode "%~dp0\\<filename>.js" %*), write it into
tmpDir alongside the .js and ensure both files are created (use the existing
fullPath and filename to derive the shim name), so Windows CI won't rely on
system-level .js associations; keep existing permissions behavior for the .js
file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isScriptPathSafepath-traversal guard andrunShellScriptspawn helper (executes directly so the script's shebang picks the interpreter — fixes bash/zsh compatibility)parseEnvVarsFromOutputto injectKEY=VALUEstdout lines into the active environmentrenderer:run-collection-scriptIPC handler with streaming output viamain:collection-script-outputbeforeCollectionRunhook into the collection runnerbruno.jsonschema with acollectionScriptsfieldSecurity
All execution is gated behind
jsSandboxMode: developer. Scripts are spawned directly (not via a shell) so the shebang controls the interpreter; no arbitrary shell injection surface.Test plan
yarn testpasses inpackages/bruno-electron#!/bin/bashshebang executes correctly (not via/bin/zsh)../../evil.sh) is rejected byisScriptPathSafeKEY=VALUEstdout lines appear in the active environment after run🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Security
Tests